fix: scope LLM queue list to authenticated user (#508)#529
Conversation
…t cross-user data leak (#508)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-Review (Adversarial Pass)Categories checked:
No additional issues found. Fix is complete. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces user-based isolation for LLM queue operations, ensuring that users can only access and view statistics for their own requests. Key changes include updating the ILlmQueueService and ILlmQueueRepository to include userId in status and statistics queries, implementing a new GetByUserAndStatusAsync repository method, and adding integration tests to verify data isolation. Feedback focuses on optimizing the GetQueueStatsAsync implementation to avoid multiple database calls and improving query performance by using non-tracking queries for read-only operations.
| var pending = await _unitOfWork.LlmQueue.GetByUserAndStatusAsync(userId, RequestStatus.Pending); | ||
| var processing = await _unitOfWork.LlmQueue.GetByUserAndStatusAsync(userId, RequestStatus.Processing); | ||
| var completed = await _unitOfWork.LlmQueue.GetByUserAndStatusAsync(userId, RequestStatus.Completed); | ||
| var failed = await _unitOfWork.LlmQueue.GetByUserAndStatusAsync(userId, RequestStatus.Failed); |
There was a problem hiding this comment.
This implementation is inefficient as it makes four separate database calls to get the counts for each status. Each call fetches a list of full LlmRequest entities, which is unnecessary when only the count is needed.
This can be optimized to a single database query. Consider adding a new method to the ILlmQueueRepository that uses a GROUP BY clause to fetch all status counts for a user in one go.
For example, a new repository method could execute a query similar to this:
var statusCounts = await _context.LlmRequests
.Where(r => r.UserId == userId)
.GroupBy(r => r.Status)
.Select(g => new { Status = g.Key, Count = g.Count() })
.ToDictionaryAsync(g => g.Status, g => g.Count);This service method would then just call the new repository method and construct the QueueStatsDto from the returned dictionary. This would be much more performant.
There was a problem hiding this comment.
Fixed: replaced the four GetByUserAndStatusAsync calls with a new GetStatusCountsByUserAsync repository method that uses a single GROUP BY query. The new method returns Dictionary<RequestStatus, int> and GetQueueStatsAsync now builds the DTO from it with .GetValueOrDefault(status, 0) per status.
| public async Task<IEnumerable<LlmRequest>> GetByUserAndStatusAsync(Guid userId, RequestStatus status, CancellationToken cancellationToken = default) | ||
| { | ||
| if (_context.Database.IsSqlite()) | ||
| { | ||
| return await _context.LlmRequests | ||
| .FromSqlInterpolated($"SELECT * FROM LlmRequests WHERE UserId = {userId} AND Status = {(int)status} ORDER BY CreatedAt DESC") | ||
| .Include(lr => lr.Board) | ||
| .ToListAsync(cancellationToken); | ||
| } | ||
|
|
||
| return await _context.LlmRequests | ||
| .Include(lr => lr.Board) | ||
| .Where(lr => lr.UserId == userId && lr.Status == status) | ||
| .OrderByDescending(lr => lr.CreatedAt) | ||
| .ToListAsync(cancellationToken); | ||
| } |
There was a problem hiding this comment.
Since this method is used for read-only operations where the entities are mapped to DTOs, it would be more efficient to disable change tracking by adding .AsNoTracking() to the query. This avoids the overhead of setting up change tracking for entities that won't be updated.
public async Task<IEnumerable<LlmRequest>> GetByUserAndStatusAsync(Guid userId, RequestStatus status, CancellationToken cancellationToken = default)
{
if (_context.Database.IsSqlite())
{
return await _context.LlmRequests
.FromSqlInterpolated($"SELECT * FROM LlmRequests WHERE UserId = {userId} AND Status = {(int)status} ORDER BY CreatedAt DESC")
.AsNoTracking()
.Include(lr => lr.Board)
.ToListAsync(cancellationToken);
}
return await _context.LlmRequests
.AsNoTracking()
.Include(lr => lr.Board)
.Where(lr => lr.UserId == userId && lr.Status == status)
.OrderByDescending(lr => lr.CreatedAt)
.ToListAsync(cancellationToken);
}There was a problem hiding this comment.
Fixed: added .AsNoTracking() to both SQLite and LINQ branches of GetByUserAndStatusAsync. Also added .Include(lr => lr.User) for consistency with GetByStatusAsync.
Adversarial Review (Independent Pass)Verdict: APPROVE (with one NOTE worth tracking)Findings1. All LlmQueueController endpoints scoped — PASS with one pre-existing note The three list-facing endpoints (
2. UserId claim extraction safe on missing claim — PASS
3. Repository method used correctly everywhere — PASS with NOTE
No unscoped call survives in a user-facing code path. NOTE (cosmetic): 4. Tests use distinct users and non-vacuous assertions — PASS
Both tests use distinct 5. Worker path intentionally unscoped and justified — PASS
6. No compile errors introduced — PASS
7. Tests pass — PASS
Categories checked
One actionable note (not a blocker)The |
Adds a single GROUP BY method returning Dictionary<RequestStatus, int> to replace the four-call pattern in GetQueueStatsAsync.
…Repository Adds GROUP BY implementation for GetStatusCountsByUserAsync. Adds AsNoTracking() to both SQLite and LINQ branches of GetByUserAndStatusAsync to eliminate EF change-tracking overhead. Also adds Include(lr => lr.User) for consistency with GetByStatusAsync.
Uses new GetStatusCountsByUserAsync repository method to fetch all status counts in one round-trip. Builds QueueStatsDto from the returned dictionary using GetValueOrDefault(status, 0) per status key.
Updates GetQueueStatsAsync_ShouldReturnCorrectCounts to use the new single-query method instead of four GetByUserAndStatusAsync stubs.
Gemini Feedback AddressedBoth inline comments from Gemini have been resolved: [HIGH] GetQueueStatsAsync — single GROUP BY query [MEDIUM] AsNoTracking on GetByUserAndStatusAsync Build: clean. LlmQueue tests: all pass. |
1 participant
Summary
UserIdpredicate toGET /api/llm-queue/status/{status}andGET /api/llm-queue/statsso each user only sees their own queue itemsRoot Cause
GetByStatusandGetQueueStatsendpoints fetched records without filtering by the calling user's identity. TheGetUserQueueendpoint was already correct; the status and stats endpoints were not.Fix
GetByUserAndStatusAsync(Guid userId, RequestStatus status)replaces the unscopedGetByStatusAsynccall in user-facing service methodsGetQueueByStatusAsyncandGetQueueStatsAsyncinLlmQueueServicenow require and use auserIdparameteruserIdviaTryGetCurrentUserId(theAuthenticatedControllerBasepattern already established) before calling the serviceProcessNextRequestAsyncis intentionally left unscoped — it is a system/worker operation, not a user-facing listTests
GetByStatus_ShouldOnlyReturnCurrentUserRequests: user A creates a pending item; user B queries Pending status and must not see itGetQueueStats_ShouldOnlyCountCurrentUserRequests: user A creates an item; user B'sPendingCountmust not increaseGetByUserAndStatusAsyncmock signatures (no behavioral change, just signature alignment)Risk
Low — additive WHERE clause; no schema changes; no behaviour change for single-user setups.